Remove wheel-tests-cudf-polars-with-rapidsmpf in favor of existing wheel-tests-cudf-polars#22467
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughConsolidates cudf_polars experimental testing into main wheel test workflow: removes separate experimental scripts and job, adds container runtime options to wheel-tests-cudf-polars, enables tests/experimental in CI runs, and adds the ChangesTest Workflow Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jameslamb
left a comment
There was a problem hiding this comment.
Approving so you can do what you want, left one small question/suggestion.
| --prefer-binary \ | ||
| --constraint "${PIP_CONSTRAINT}" \ | ||
| "$(echo "${CUDF_POLARS_WHEELHOUSE}"/cudf_polars_"${RAPIDS_PY_CUDA_SUFFIX}"*.whl)[test,experimental]" \ | ||
| "$(echo "${CUDF_POLARS_WHEELHOUSE}"/cudf_polars_"${RAPIDS_PY_CUDA_SUFFIX}"*.whl)[test,experimental,ray]" \ |
There was a problem hiding this comment.
With this change, is there ANY place left in CI where cudf-polars is tested without ray?
If not, then CI can't catch a bug of the form "we call ray an optional dependency but it's imported unconditionally".
Might be worth coming up with a plan for that. The lightest-weight way I can think of (but which isn't as thorough as running the test suite) would be to do something like this:
pip uninstall --yes ray
python -c "import cudf_polars"There was a problem hiding this comment.
Good observation. We should be running the Polars test suite (cudf-polars-polars-tests) without Ray (or Dask) installed, but it feel less "intentional" to rely on integrations tests to validate that these dependencies are optional. I opened #22478 to follow up on this
|
/merge |
Description
Now that rapidsmpf is a required dependency of cudf_polars,
wheel-tests-cudf-polars-with-rapidsmpfwas almost functionally identical towheel-tests-cudf-polars. To make the former align to the latter:wheel-tests-cudf-polarsjob--ignore=tests/experimentalwas removed when running tests inwheel-tests-cudf-polarsChecklist